Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement kernel compilation in the assembler #1418

Merged
merged 198 commits into from
Jul 30, 2024
Merged

Conversation

bobbinth
Copy link
Contributor

@bobbinth bobbinth commented Jul 29, 2024

This PR builds on #1401 and adds ability to assemble kernel libraries. Specifically:

  • Added Assembler::assemble_kernel() method which takes a single module and returns a compiled library. We can extend this in the future to take a list of modules one of which will be the kernel module.
  • Added Assembler::with_kernel() constructor which takes a compiled library representing the kernel to be used by the assembler.
    • This also relied on the new ModuleGraph::with_kernel() constructor and the new error type AssemblerError.

Also, as a part of this PR I removed a bunch of error variants from AssemblyError which seemed to be unused.

Not done in this PR:

  • Tests for the new functionality (included tests turn off in the prior PR).

@bobbinth bobbinth marked this pull request as draft July 29, 2024 00:07
@bobbinth bobbinth marked this pull request as ready for review July 29, 2024 08:08
Copy link
Contributor

@plafer plafer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Left a few suggestions

assembly/src/errors.rs Outdated Show resolved Hide resolved
@@ -5,6 +5,9 @@ use crate::{
use alloc::vec::Vec;
use miden_crypto::hash::rpo::RpoDigest;

// KERNEL
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't it cleaner to only have sections when there are more than 1 definitions in a file?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had the same thought, kinda just feels like clutter when there is nothing else in the file except the item in question.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I usually put these in to separate the "imports" section from the main code. It is a kind of a visual clue for "important code starts after this line".

assembly/src/ast/module.rs Outdated Show resolved Hide resolved
assembly/src/assembler/mod.rs Outdated Show resolved Hide resolved
assembly/src/assembler/mod.rs Outdated Show resolved Hide resolved
assembly/src/assembler/module_graph/mod.rs Show resolved Hide resolved
assembly/src/assembler/mod.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@bitwalker bitwalker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, looks good! I left a few comments where I think we should consider some small changes, hence why I'm marking this as requesting changes, but if there's pushback on those, let me know. Once I've caught up again later today, I'll re-review.

Base automatically changed from plafer-mast-library-compilation to next July 29, 2024 18:32
Copy link
Contributor

@plafer plafer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a suggestion, which we can implement in a subsequent PR if we decide to go down that route

Comment on lines +107 to +109
kernel: Kernel,
kernel_info: ModuleInfo,
library: CompiledLibrary,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking a KernelLibrary would only wrap a CompiledLibrary (and would make all its checks in the constructor). Then the ModuleInfo and Kernel can be derived from the CompiledLibrary in methods. The advantage of that approach would be that the struct would be simpler to understand, since it wouldn't store any duplicate information.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is how I had it originally but then noticed that the constructor and into_parts() method were almost identical (e.g., we need to build a Kernel at construction time to validate that it can be built without errors). So, given that additional memory footprint is likely to be minimal (kernels can have at most 256 procedures), decided not to throw out all the stuff we built in the constructor and save it with the struct.

I do agree that we could probably refactor things a bit here (e.g., maybe Kernel should be just a newtype over ModuleInfo) but I'm leaving this to future PRs.

@bobbinth bobbinth merged commit afbd43c into next Jul 30, 2024
9 checks passed
@bobbinth bobbinth deleted the bobbin-assemble-kernel branch July 30, 2024 16:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants